added ignore module pattern to calculate the first semester, solves #95 #97
Conversation
|
Tried out the changes and works for me in chrome 👍 |
| /** | ||
| * list of modules that should be ignored to calculate the first semester | ||
| * check if moduleName matches ignore pattern | ||
| */ | ||
| shouldModuleBeIgnored: (hsluModuleName) => { | ||
| const ignoreModulePatterns = ['INFO_ABEND']; | ||
| let dontIgnore = true; | ||
| ignoreModulePatterns.forEach(ignorePattern => { | ||
| if(hsluModuleName.includes(ignorePattern)) { | ||
| console.log(`ignoring module ${hsluModuleName} for first semester calculation`) | ||
| dontIgnore = false; | ||
| } | ||
| }) | ||
|
|
||
| return dontIgnore; | ||
| }, |
There was a problem hiding this comment.
Bool variables should always have a positive name, e.g. isTrue rather than isNotTrue. Otherwise the valid state is represented as a double negation as in this case with dontIgnore = false meaning the module is invalid. This is simply very confusing.
Besides that, the method name doesn't correspond to what is actually returned. With a name like shouldModuleBeIgnored, I'd expect the method to return true if it is ignored and otherwise false. But it does exactly the opposite.
I'd suggest to change the method like so to make it more easy to understand:
| /** | |
| * list of modules that should be ignored to calculate the first semester | |
| * check if moduleName matches ignore pattern | |
| */ | |
| shouldModuleBeIgnored: (hsluModuleName) => { | |
| const ignoreModulePatterns = ['INFO_ABEND']; | |
| let dontIgnore = true; | |
| ignoreModulePatterns.forEach(ignorePattern => { | |
| if(hsluModuleName.includes(ignorePattern)) { | |
| console.log(`ignoring module ${hsluModuleName} for first semester calculation`) | |
| dontIgnore = false; | |
| } | |
| }) | |
| return dontIgnore; | |
| }, | |
| /** | |
| * Check if a module should be included in the first semester calculation. | |
| */ | |
| isModuleValid: (hsluModuleName) => { | |
| const ignoreModulePatterns = ['INFO_ABEND']; | |
| let isValid = true; | |
| ignoreModulePatterns.forEach(ignorePattern => { | |
| if (moduleName.includes(ignorePattern)) { | |
| console.log(`ignoring module ${hsluModuleName} for first semester calculation`) | |
| isValid = false; | |
| } | |
| }) | |
| return isValid; | |
| }, |
| @@ -157,6 +173,8 @@ const ModuleParser = { | |||
| firstModule = anlasslistApiResponse.items | |||
| .slice() | |||
| .reverse() | |||
| //check if module is is in the ignore list | |||
There was a problem hiding this comment.
no need to add a comment if the method name and docs already explain what's going on :)
| let dontIgnore = true; | ||
| ignoreModulePatterns.forEach(ignorePattern => { | ||
| if(hsluModuleName.includes(ignorePattern)) { | ||
| console.log(`ignoring module ${hsluModuleName} for first semester calculation`) | ||
| dontIgnore = false; | ||
| } | ||
| }) | ||
|
|
||
| return dontIgnore; |
There was a problem hiding this comment.
| let dontIgnore = true; | |
| ignoreModulePatterns.forEach(ignorePattern => { | |
| if(hsluModuleName.includes(ignorePattern)) { | |
| console.log(`ignoring module ${hsluModuleName} for first semester calculation`) | |
| dontIgnore = false; | |
| } | |
| }) | |
| return dontIgnore; | |
| return ignoreModulePatterns.every(pattern => !hsluModuleName.includes(pattern)); |
There was a problem hiding this comment.
I like this solution even better 👍
I added a new static list of module name patterns that should be ignored on the first semester calculation.
These are only ignored on the
firstModulecalculation.This solves the issue #95.
If you want another solution I'm open to it.
Maybe we should also ignore modules that are in
ignoreInStatsModules.